Skip to content

Update PiTFT overlays for compatibility and consistency #5903

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 31, 2024

Conversation

makermelissa
Copy link

@makermelissa makermelissa commented Jan 26, 2024

This PR includes the following changes:

  • Touch parameters now exposed on resistive touchscreens (2.8 and 3.5)
  • DRM parameter added for pitft22 and pitft28-capacitive overlays like in resistive overlays
  • "multi-inno,mi0283qt" changed to "adafruit,yx240qv29" because the rotations in that driver are consistent with the fbtft driver.

cc: @ladyada

Tested with the corresponding hardware on a Raspberry Pi 5.

Copy link
Contributor

@6by9 6by9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates. Just a few minor niggles to be addressed.

@@ -43,7 +43,7 @@
status = "okay";

pitft: pitft@0{
compatible = "ilitek,ili9340";
compatible = "ilitek,ili9340", "adafruit,yx240qv29";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I think was probably my error in the pitft35-resistive overlay.
"ilitek,ili9340" is the fbtft compatible. "adafruit,yx240qv29" is the tinydrm one. We already have the DRM override rewriting this correctly, so the original line here is correct.

@@ -42,7 +42,7 @@
#size-cells = <0>;

pitft: pitft@0{
compatible = "ilitek,ili9340";
compatible = "ilitek,ili9340", "adafruit,yx240qv29";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto

fps = <&pitft>,"fps:0";
debug = <&pitft>,"debug:0";
drm = <&pitft>,"compatible=adafruit,yx240qv29";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This override needs documenting in README.

@@ -49,7 +49,7 @@
#size-cells = <0>;

pitft: pitft@0{
compatible = "ilitek,ili9340", "multi-inno,mi0283qt";
compatible = "ilitek,ili9340", "adafruit,yx240qv29";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

, "multi-inno,mi0283qt" should be removed, and only the override updated.

@@ -64,6 +64,9 @@
};

pitft_ts@1 {
/* needed to avoid dtc warning */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use tabs to indent. (That should be the case in the other 2 files, but at least they are self-consistent in using spaces throughout).

fps = <&pitft>,"fps:0";
debug = <&pitft>,"debug:0";
drm = <&pitft>,"compatible=multi-inno,mi0283qt";
speed = <&pitft>,"spi-max-frequency:0";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again tabs for indentation.

touch-sizey = <&stmpe_touchscreen>,"touchscreen-size-y?";
touch-invx = <&stmpe_touchscreen>,"touchscreen-inverted-x?";
touch-invy = <&stmpe_touchscreen>,"touchscreen-inverted-y?";
touch-swapxy = <&stmpe_touchscreen>,"touchscreen-swapped-x-y?";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New overrides need documenting in README

@@ -64,6 +64,9 @@
};

pitft_ts@1 {
/* needed to avoid dtc warning */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indentation.

touch-sizey = <&stmpe_touchscreen>,"touchscreen-size-y?";
touch-invx = <&stmpe_touchscreen>,"touchscreen-inverted-x?";
touch-invy = <&stmpe_touchscreen>,"touchscreen-inverted-y?";
touch-swapxy = <&stmpe_touchscreen>,"touchscreen-swapped-x-y?";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation.

@makermelissa
Copy link
Author

Thanks, changes updated. I also added the drm parameter to the readme on the 2.8" capacitive and 2.2" displays.

@6by9
Copy link
Contributor

6by9 commented Jan 29, 2024

There is one line left with spaces for indentation

WARNING: please, no spaces at the start of a line
#76: FILE: arch/arm/boot/dts/overlays/pitft22-overlay.dts:10:
+    compatible = "brcm,bcm2835";$

Up to @pelwell whether he's fussed over the spaces. Overlays will never get upstreamed, so it's not as critical.

Note 1: Generally we want the commits as they are to be merged, so fixups should be squashed into the base patch, or use git commit --amend to modify it. You then need git push -f ... when pushing to update the branch on Github.
We can squash all commits of a PR into one which would work in this case, but that's not always the desired result.

Note 2: any patches to drivers or anything that may need to be upstreamed really need to have a Signed-off-by: line. See https://www.kernel.org/doc/html/latest/process/submitting-patches.html#sign-your-work-the-developer-s-certificate-of-origin for what that means, but it can be done simply by using git commit -s when generating your patch.

@makermelissa
Copy link
Author

makermelissa commented Jan 29, 2024

Oh yeah, thanks. Spacing fixed now.
Is it too late to combine the previous commits?

@makermelissa
Copy link
Author

Ok, signed-off by line added too.

@6by9
Copy link
Contributor

6by9 commented Jan 29, 2024

Oh yeah, thanks. Fixed now. Is it too late to combine the previous commits?

Nothing has been merged yet, so you can modify your branch as much as you want to reflect what you are proposing to merge. You can therefore use git rebase -i or similar to edit your branch, squash things together, alter commit messages, etc, and then push an updated branch.

@makermelissa
Copy link
Author

Ok, had to do it with a case-sensitive filesystem, but success.

@makermelissa
Copy link
Author

Added commit description because it was failing without one.

@pelwell
Copy link
Contributor

pelwell commented Jan 30, 2024

All the touch-size parameters are targeting integer properties, but they're written as if they're targeting booleans - you need:

		touch-sizex = <&ft6236>,"touchscreen-size-x:0";

Also, it bothers me that the documentation says the sizes have default values, but those defaults are not present in the overlay - the stmpe_touchscreen nodes don't seem to include any of the touchscreen- properties unless the parameters are used.

  1. The fact that the various size properties are broken makes me think this hasn't been tested. Are you sure that the touchscreen- properties are recognised by the driver?
  2. Are the defaults quoted in the README correct?

@makermelissa
Copy link
Author

makermelissa commented Jan 30, 2024

I'm sure that at least the invert and swap ones are recognized as I have modified those. They are part of linux/input/touchscreen.h and I believe they are read when touchscreen_parse_properties() is called.

@pelwell
Copy link
Contributor

pelwell commented Jan 30, 2024

And are the default sizes as given in the README?

@makermelissa
Copy link
Author

Sorry, was looking it up still. The defaults for 3.5" should be 320 x 480. However, I think they may not be so useful when applied to the resistive touchscreens, so I could remove those. I really only needed the inverts and swap params.

@makermelissa
Copy link
Author

Ok, updated the 3.5" size at least.

@makermelissa
Copy link
Author

Ok, I see the capacitive one (which I got them from) has defaults, but not the new ones. I should either add default sizes to the overlays or remove them. What do you think?

@makermelissa
Copy link
Author

Actually, I'll test to see if adding the defaults messes up anything and if so, I'll just remove them. If not, I'll see if changing has any effect. If not, I'll remove them.

@pelwell
Copy link
Contributor

pelwell commented Jan 30, 2024

I'm happy for you to use any defaults or not, as you wish, provided it works as described.

Expose the invert and swap touch parameters on 2.8" and 3.5" resistive touchscreens. Add
the DRM parameter to the PiTFT 2.2" and 2.8" Capacitive overlay in the same
way it is on the resistive overlays. Change the DRM driver to `adafruit,yx240qv29`
because the rotations are consistent with the FBTFT Driver. Fix the override size parameters
on the 2.8" capacitive PiTFT.

Signed-off-by: Melissa LeBlanc-Williams <[email protected]>
@makermelissa
Copy link
Author

Ok, was getting some strange results on the Resistive PiTFTs (negative values) with defaults added, so I removed the size ones. I fixed the overrides on the capacitive display and updated the README to reflect those changes.

@pelwell
Copy link
Contributor

pelwell commented Jan 30, 2024

That looks better (on a phone, will check properly before merging when the build checks complete). Don't worry about the commit description line length - it will get squashed into the monolithic DT commit anyway.

@pelwell pelwell merged commit 1c2a93c into raspberrypi:rpi-6.1.y Jan 31, 2024
@pelwell
Copy link
Contributor

pelwell commented Jan 31, 2024

Thanks!

popcornmix added a commit to raspberrypi/firmware that referenced this pull request Feb 8, 2024
See: raspberrypi/linux#5923

kernel: overlays: Delete deprecated overlay mpu6050

kernel: overlays: Correct some compatible strings

kernel: ASoC: DACplusADCPro - fix 16bit sample support in clock consumer mode
See: raspberrypi/linux#5919

kernel: ASoC: adds support for Hifiberry AMP4Pro to the dacplus driver
See: raspberrypi/linux#5918

kernel: ASoC: DACplus - fix 16bit sample support in clock consumer mode
See: raspberrypi/linux#5917

kernel: Improve I2C timing (again)
See: raspberrypi/linux#5916

kernel: Update PiTFT overlays for compatibility and consistency
See: raspberrypi/linux#5903

kernel: Support non-standard I2C timings on Pi 5
See: raspberrypi/linux#5853

kernel: overlays: Add pcie-32bit-dma-pi5-overlay to enable 32bit DMA on the Pi 5's external PCIe interface
See: raspberrypi/linux#5897

kernel: Improvement on backup-switchover-mode overlay value definitions
See: raspberrypi/linux#5884

kernel: Pisound updates for Pi 5
See: raspberrypi/linux#5872
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Feb 8, 2024
See: raspberrypi/linux#5923

kernel: overlays: Delete deprecated overlay mpu6050

kernel: overlays: Correct some compatible strings

kernel: ASoC: DACplusADCPro - fix 16bit sample support in clock consumer mode
See: raspberrypi/linux#5919

kernel: ASoC: adds support for Hifiberry AMP4Pro to the dacplus driver
See: raspberrypi/linux#5918

kernel: ASoC: DACplus - fix 16bit sample support in clock consumer mode
See: raspberrypi/linux#5917

kernel: Improve I2C timing (again)
See: raspberrypi/linux#5916

kernel: Update PiTFT overlays for compatibility and consistency
See: raspberrypi/linux#5903

kernel: Support non-standard I2C timings on Pi 5
See: raspberrypi/linux#5853

kernel: overlays: Add pcie-32bit-dma-pi5-overlay to enable 32bit DMA on the Pi 5's external PCIe interface
See: raspberrypi/linux#5897

kernel: Improvement on backup-switchover-mode overlay value definitions
See: raspberrypi/linux#5884

kernel: Pisound updates for Pi 5
See: raspberrypi/linux#5872
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants